Introduce CLI flag -environment #15422

urso · 2020-01-09T15:48:52Z

Type of change

Bug fix

What does this PR do?

Introduce CLI flags -environment to control the default logging settings
the Beat should use if no logging is configured. The behavior of -e does
not change. By replacing -e with -environment system in the system
unit file we continue to log to stdout/stderr by default, but users are
still able to overwrite settings.

For now (I'm targeting 7.x) I tried to not introduce any breaking changes one way or the other on the config file or CLI level. So the idea is to replace the -e flag with the -environment flag in the systemd unit file. If -environment systemd is given the default log output is stdout/stderr, while we keep file based logging as default for other environments.

Why is it important?

If beats are started via systemd unit files the -e CLI flag is added to the list of CLI arguments. This was introduced in #8942. The -e flag its purpose is to disable the configured logging output and always log to stdout/stderr. By to enforcing the -e flag the users logging configurations is not honored (for example see the reports in #12024).

Checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation

How to test this PR locally

The PR changes how beats are started as Windows Service, macOS service (installed via launchctl), docker container, and via systemd (deb/rpm). For testing Beats must be installed in a few different environment types to check if logs are correctly written to the default target and if log settings can be overwritten.

Running beat locally via command line:

Running a beat (e.g. metricbeat with system module) locally without configuring logging output should log to stdout/stderr if -environment systemd is given on the command line
Running beat without -environment flag, without logging configured should create log files as usual

Running a beat via systemd:

The Beat should log to journald by default
Logs should be written to /var/log/<beatname> when setting logging.to_stderr: false and logging.to_file: true

Running beat using old init scripts:

Install Beat on system still using init scripts
Logs should be written to /var/log/<beatname> by default

Running as container:

The Beat should log to stdout/stderr by default. Check via docker log is this is really the case.
Configure to_file in the config file and log to a mounted directory. Check the Beat now writes logs to the configured path.

Running on windows

Install the windows package zip file on windows:
- check the service is correctly installed
- check the service can be started
- check the service writes logs to the configured files

Running as service on macOS

Install the packaged Beat on macOS. Including launchd files and preference pane:
- check the service is correctly installed and can be restarted
- check the service writes logs to the configured files

Related issues

libbeat/logp/environment.go

urso · 2020-01-09T15:51:12Z

@jsoriano @andrewkroh Some idea for fixing the logging configuration not being ignored when running via systemd. WDYT?

jsoriano · 2020-01-09T18:35:30Z

@jsoriano @andrewkroh Some idea for fixing the logging configuration not being ignored when running via systemd. WDYT?

I think this can be a good approach for the issues raised in #12024 👍

jsoriano · 2020-01-09T18:39:12Z

We will also need to update docs where we mention the default value for BEAT_LOG_OPTS in systemd.

graphaelli · 2020-01-16T16:00:04Z

cc @elastic/apm-server

jsoriano

It mostly LGTM, I have added some minor comments. Thanks for addressing this issue!

dev-tools/packaging/templates/darwin/launchd-daemon.plist.tmpl

libbeat/logp/config.go

libbeat/logp/environment.go

jsoriano · 2020-01-16T16:09:54Z

libbeat/logp/configure/logging.go

 func init() {
 	flag.BoolVar(&verbose, "v", false, "Log at INFO level")
 	flag.BoolVar(&toStderr, "e", false, "Log to stderr and disable syslog/file output")
 	common.StringArrVarFlag(nil, &debugSelectors, "d", "Enable certain debug selectors")
+	flag.Var((*environmentVar)(&environment), "environment", "set environment the Beat is run in")


We should add docs about this new flag.

It is kind of a private flag (a hack) I just introduced as a workaround. I rather wish to remove the flag again in the future. Maybe for 8.0 we have to rethink some logging defaults. e.g. maybe we can detect the environment we are running in, or just log to stdout/stderr by default.

Yep, I think we could consider logging to stdout/stderr by default on 8.0.

jsoriano · 2020-01-16T17:22:56Z

libbeat/logp/configure/logging.go

 func init() {
 	flag.BoolVar(&verbose, "v", false, "Log at INFO level")
 	flag.BoolVar(&toStderr, "e", false, "Log to stderr and disable syslog/file output")
 	common.StringArrVarFlag(nil, &debugSelectors, "d", "Enable certain debug selectors")
+	flag.Var((*environmentVar)(&environment), "environment", "set environment the Beat is run in")


Yep, I think we could consider logging to stdout/stderr by default on 8.0.

dedemorton · 2020-01-17T20:09:32Z

CHANGELOG.next.asciidoc

@@ -38,7 +38,8 @@ https://github.com/elastic/beats/compare/v7.0.0-alpha2...master[Check the HEAD d

 *Affecting all Beats*

-TLS or Beats that accept connections over TLS and validate client certificates. {pull}14146[14146]
+- TLS or Beats that accept connections over TLS and validate client certificates. {pull}14146[14146]


Looks like a line got dropped from the changelog somehow. The full text of this should be:

Fix panics that could result from invalid TLS certificates. This can affect Beats that connect over TLS, or Beats that accept connections over TLS and validate client certificates. {pull}14146[14146]

dedemorton · 2020-01-17T20:13:59Z

libbeat/docs/command-reference.asciidoc

@@ -910,6 +910,13 @@ messages.
 *`-e, --e`*::
 Logs to stderr and disables syslog/file output.

+*`-environment`*::
+Tell {beatname_uc} the environment it runs into. If no log output is configured


It's not clear up front that this setting relates to logging. Here's my suggested edit based on what I think you're saying here:

For logging purposes, specifies the environment that {beatname_uc} is running in. This setting is used to select a default log output when no log output is configured. Supported values are: `systemd`, `container`, `macos_service`, and `windows_service`. If `systemd` or `container` is specified, {beatname_uc} will log to stdout and stderr by default.

Thanks, will update. Initially I didn't want to this setting to be about logging only. A generic flag like environment is not really bound to logging. Other subsystems could also be configured differently based on the environment. On the other hand I'd prefer to not have this flag at all...

dedemorton

LGTM. We can always revisit the description later on if the -environment option stays around and gets used for other things. :-)

urso · 2020-01-20T15:34:08Z

Jenkins, test this.

urso · 2020-01-21T23:36:52Z

Jenkins, test this.

Add windows_service, macos_service, and container envionment types. If beats are installed using our scripts the `-environment` flag will always be set. Replace `-e` with `-environment docker` CLI flag in the Dockerfile. Although it's uncommon, users can not overwrite the logging configuration in docker containers as well. The docker(container) environment and systemd environment will default to stdout/stderr logging. All other environments continue to use file rotation as default.

* Introduce CLI flag -environment Introduce CLI flags -environment to control the default logging settings the Beat should use if no logging is configured. The behavior of -e does not change. By replacing `-e` with `-environment system` in the system unit file we continue to log to stdout/stderr by default, but users are still able to overwrite settings. * Add more environment types Add windows_service, macos_service, and container envionment types. If beats are installed using our scripts the `-environment` flag will always be set. Replace `-e` with `-environment docker` CLI flag in the Dockerfile. Although it's uncommon, users can not overwrite the logging configuration in docker containers as well. The docker(container) environment and systemd environment will default to stdout/stderr logging. All other environments continue to use file rotation as default. (cherry picked from commit 1d3d4d1)

* Introduce CLI flag -environment (#15422) * Introduce CLI flag -environment Introduce CLI flags -environment to control the default logging settings the Beat should use if no logging is configured. The behavior of -e does not change. By replacing `-e` with `-environment system` in the system unit file we continue to log to stdout/stderr by default, but users are still able to overwrite settings. * Add more environment types Add windows_service, macos_service, and container envionment types. If beats are installed using our scripts the `-environment` flag will always be set. Replace `-e` with `-environment docker` CLI flag in the Dockerfile. Although it's uncommon, users can not overwrite the logging configuration in docker containers as well. The docker(container) environment and systemd environment will default to stdout/stderr logging. All other environments continue to use file rotation as default. (cherry picked from commit 1d3d4d1)

houndci-bot reviewed Jan 9, 2020

View reviewed changes

libbeat/logp/environment.go Show resolved Hide resolved

libbeat/logp/environment.go Show resolved Hide resolved

libbeat/logp/environment.go Show resolved Hide resolved

urso added bug in progress Pull request is currently in progress. libbeat Team:Beats labels Jan 9, 2020

urso mentioned this pull request Jan 9, 2020

Add a pull request template providing valuable information when reviewing a PR #15388

Merged

5 tasks

jsoriano added the test-plan Add this PR to be manual test plan label Jan 9, 2020

mdelapenya mentioned this pull request Jan 13, 2020

Discussion for enriching the PR template elastic/apm-agent-java#990

Closed

18 tasks

urso added the v7.7.0 label Jan 16, 2020

urso force-pushed the environment-based-logging branch from 13f090c to 45540f2 Compare January 16, 2020 15:19

urso changed the title ~~[WIP] Introduce CLI flag -environment~~ Introduce CLI flag -environment Jan 16, 2020

urso marked this pull request as ready for review January 16, 2020 15:38

urso added review [zube]: In Review needs_backport PR is waiting to be backported to other branches. and removed in progress Pull request is currently in progress. labels Jan 16, 2020

jsoriano reviewed Jan 16, 2020

View reviewed changes

urso requested a review from dedemorton January 16, 2020 16:54

jsoriano approved these changes Jan 16, 2020

View reviewed changes

urso force-pushed the environment-based-logging branch from d5d0ded to 0063bbb Compare January 16, 2020 21:10

urso added the docs label Jan 17, 2020

dedemorton requested changes Jan 17, 2020

View reviewed changes

dedemorton approved these changes Jan 18, 2020

View reviewed changes

urso force-pushed the environment-based-logging branch from 992c4df to fe92cd5 Compare January 20, 2020 13:50

urso added 14 commits January 24, 2020 16:34

always pass -environment=systemd

36d5085

Add flag to command root

fd0cc10

Update docs

31cdfdd

Add changelog

b9731b4

Feed the hound

fc556fe

review

00d9a5f

Document -environment CLI flag

744f3a0

unify indentation to 4 spaces

3a44428

fix changelog

1d6b502

Fix changelog

1aa6c1e

Update docs

c070704

hmm...

3567069

fix changelog... again

64aff4c

urso force-pushed the environment-based-logging branch from 14d3ff4 to 64aff4c Compare January 24, 2020 15:34

urso merged commit 1d3d4d1 into elastic:master Jan 24, 2020

urso deleted the environment-based-logging branch January 24, 2020 15:35

zube bot added [zube]: Done and removed [zube]: In Review labels Jan 24, 2020

urso mentioned this pull request Jan 24, 2020

Cherry-pick #15422 to 7.x: Introduce CLI flag -environment #15829

Merged

4 tasks

urso removed the needs_backport PR is waiting to be backported to other branches. label Jan 24, 2020

andresrc removed the [zube]: Done label Jan 27, 2020

andresrc added the Team:Integrations Label for the Integrations team label Mar 6, 2020

andresrc added the test-plan-added This PR has been added to the test plan label Mar 21, 2020

urso mentioned this pull request Mar 30, 2020

Populate default settings for logging.file.* #17332

Merged

1 task

simitt mentioned this pull request Mar 30, 2020

Starting APM Server with -environment container fails elastic/apm-server#3576

Closed

This was referenced Mar 31, 2020

Cherry-pick #17332 to 7.x: Populate default settings for logging.file.* #17369

Merged

Cherry-pick #17332 to 7.7: Populate default settings for logging.file.* #17370

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce CLI flag -environment #15422

Introduce CLI flag -environment #15422

urso commented Jan 9, 2020 •

edited by zube bot

Loading

urso commented Jan 9, 2020

jsoriano commented Jan 9, 2020

jsoriano commented Jan 9, 2020

graphaelli commented Jan 16, 2020

jsoriano left a comment

jsoriano Jan 16, 2020

urso Jan 16, 2020

jsoriano Jan 16, 2020

jsoriano Jan 16, 2020

dedemorton Jan 17, 2020

dedemorton Jan 17, 2020

urso Jan 18, 2020

dedemorton left a comment

urso commented Jan 20, 2020

urso commented Jan 21, 2020

Introduce CLI flag -environment #15422

Introduce CLI flag -environment #15422

Conversation

urso commented Jan 9, 2020 • edited by zube bot Loading

Type of change

What does this PR do?

Why is it important?

Checklist

How to test this PR locally

Running beat locally via command line:

Running a beat via systemd:

Running beat using old init scripts:

Running as container:

Running on windows

Running as service on macOS

Related issues

urso commented Jan 9, 2020

jsoriano commented Jan 9, 2020

jsoriano commented Jan 9, 2020

graphaelli commented Jan 16, 2020

jsoriano left a comment

Choose a reason for hiding this comment

jsoriano Jan 16, 2020

Choose a reason for hiding this comment

urso Jan 16, 2020

Choose a reason for hiding this comment

jsoriano Jan 16, 2020

Choose a reason for hiding this comment

jsoriano Jan 16, 2020

Choose a reason for hiding this comment

dedemorton Jan 17, 2020

Choose a reason for hiding this comment

dedemorton Jan 17, 2020

Choose a reason for hiding this comment

urso Jan 18, 2020

Choose a reason for hiding this comment

dedemorton left a comment

Choose a reason for hiding this comment

urso commented Jan 20, 2020

urso commented Jan 21, 2020

urso commented Jan 9, 2020 •

edited by zube bot

Loading